-
-
Notifications
You must be signed in to change notification settings - Fork 806
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dev/core#4213 Make frontend_title, name required for Group entity #26546
Conversation
(Standard links)
|
138e389
to
27fc596
Compare
Fails are because we need to call the parent |
75f450a
to
03ba6fa
Compare
This also removes a bunch of tests that are redundant due to the various other levels of api & entity testing & does other minor cleanup like fixing expected & actual variables to be in the right order A cause of test fails for civicrm#26546
This also removes a bunch of tests that are redundant due to the various other levels of api & entity testing & does other minor cleanup like fixing expected & actual variables to be in the right order A cause of test fails for civicrm#26546
This also removes a bunch of tests that are redundant due to the various other levels of api & entity testing & does other minor cleanup like fixing expected & actual variables to be in the right order A cause of test fails for civicrm#26546
19261bd
to
7f3c28b
Compare
This also removes a bunch of tests that are redundant due to the various other levels of api & entity testing & does other minor cleanup like fixing expected & actual variables to be in the right order A cause of test fails for civicrm#26546
b302870
to
9aeb31f
Compare
9aeb31f
to
872b436
Compare
872b436
to
db449b7
Compare
@eileenmcnaughton I see this is passing! Is it ready to be merged in your opinion? |
@colemanw yeah I think so - I'm on the fence about merging it now though because it another day we will cut a new rc - which means on one hand I'd have to re-do the upgrade but on the other it would get more time to find any issues |
@eileenmcnaughton are you able to move the upgrade step now that we have cut the new RC? |
UPDATE `civicrm_group` | ||
SET `frontend_title_{$locale}` = `title_{$locale}`, | ||
`frontend_description_{$locale}` = `description` | ||
WHERE `frontend_description_{$locale}` IS NULL OR `frontend_description_{$locale}` = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton doesn't this need to be 2 separate queries to handle where frontend_description is NULL or an empty string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seamuslee001 I don't think so - or rather I'm not following why it would
5a762f2
to
f5778c8
Compare
UPDATE `civicrm_group` | ||
SET `frontend_title_{$locale}` = `title_{$locale}`, | ||
`frontend_description_{$locale}` = `description` | ||
WHERE `frontend_description_{$locale}` IS NULL OR `frontend_description_{$locale}` = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton my only thought is that it should be like below where we have 2 separate queries in case frontend_description is NOT NULL i.e. what if for some reason some one had set the frontend_description but not the frontend_title this SQL wouldn't run also why don't you have the same where clause as in L21 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah got it - I pushed the change up
f5778c8
to
0950075
Compare
@@ -33,9 +33,14 @@ public function upgrade_5_64_alpha1($rev): void { | |||
$this->addTask('Drop unused civicrm_action_mapping table', 'dropTable', 'civicrm_action_mapping'); | |||
$this->addTask('Update post_URL/cancel_URL in logging tables', 'updateLogging'); | |||
$this->addTask('Add in Everybody ACL Role option value', 'addEveryBodyAclOptionValue'); | |||
$this->addTask('Fix double json encoding of accepted_credit_cards field in payment processor table', 'fixDoubleEscapingPaymentProcessorCreditCards'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton this should be kept iirc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argh
d8478be
to
ab799d6
Compare
This follows up on having done this for contribution_page recently. It will mean we can consistently refer to frontend_title in tokens and other places where what we really mean is ... frontend_title.
It now uses tokens not the now-deprecated function
ab799d6
to
e3850c0
Compare
@seamuslee001 it passed!!!!! |
ok this looks good to me lets merge and we can review |
phew - got there |
{/if} | ||
|
||
UPDATE civicrm_mailing_component | ||
SET body_html = REPLACE(body_html, '{welcome.group}', '{group.frontend_title}'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work because it tries to interpret these tokens as smarty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR: #26871
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @demeritcowboy
|
||
UPDATE `civicrm_group` | ||
SET `frontend_description_{$locale}` = `description` | ||
WHERE `frontend_description_{$locale}` IS NULL OR `frontend_description_{$locale}` = '' AND `description` <> ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My eye just caught this now too. I think it's intended to be (frontend_description_{$locale} IS NULL OR frontend_description_{$locale} = '') AND description <> ''
but it gets evaluated as frontend_description_{$locale} IS NULL OR (frontend_description_{$locale} = '' AND description <> '')
. In mysql, unlike everything else, AND has a higher precedence than OR: https://dev.mysql.com/doc/refman/8.0/en/operator-precedence.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you fixed this too - thanks!
Overview
dev/core#4213 Make frontend_title, name required for Group entity
This follows up on having done this for contribution_page recently.
It will mean we can consistently refer to frontend_title in tokens and other places where what we really mean is ... frontend_title.
Before
frontend_title, name & title optional at db level, frontend_title optional at form level, title required (name is calculated)
After
Now required at db level, frontend_title required at form level
Technical Details
I'll hit all the same problems as #26259
Comments